Skip to content

Conversation

@Adir-Shemesh
Copy link

@Adir-Shemesh Adir-Shemesh commented Aug 5, 2021

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

closes #699

for va, _, _, tinfo in vw.getImports():
# vivisect source: tinfo = "%s.%s" % (libname, impname)
modname, impname = tinfo.split(".")
modname, impname = tinfo.split(".", 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
modname, impname = tinfo.split(".", 1)
modname, _, impname = tinfo.partition(".")

Copy link
Collaborator

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this PR @Adir-Shemesh @TcM1911 and https://github.com/intezer!

I am pleasantly surprised how few changes are needed here to enable ELF support. I think we had always hoped that it would be easy to enable and I'm glad to see its the case. More importantly, the initial rule set in mandiant/capa-rules#442 makes this a valuable addition.

I don't have any issues with the code here. I think we should add a handful of regression tests to demonstrate the ELF handling works as expected. Also, there are probably a few updates needed to the documentation/readme. These things can be done in follow up commits rather than blocking this PR.

Did you notice any areas that should be further enhanced to better support ELF files?

@TcM1911
Copy link

TcM1911 commented Aug 5, 2021

Hi @williballenthin,

I'll let @Adir-Shemesh share feedback with regards to the code part.

As for the rules part, we opted to build on the already existing rules and add the needed api calls etc instead of creating new rules. This appears to be working fine. When creating new rules, we sometimes ran into issues of "Windows rules" finding matches in ELF files. Almost exclusively due to matches on POSIX functions which would be an accurate match. As you can't disable rules based on it being an ELF or PE, we merged the rules so only one of the rules is reported. By extending, it allowed for reuse of already existing "higher level" rules too. This allowed us to only create the "low level" rules such as "open socket" and "read from socket" and get "act as tcp client" for "free" etc.

A thing we have identified that would need some TLC is C++ methods. When malware use for example the C++ STD or Boost, the APIs you need to write are really ugly... The "low level" rules still detects as the methods in the end call a libc function but rules to detect reading specific files don't work as the string is in another function higher up in the call stack.

As for tests, I have submitted a test file here mandiant/capa-testfiles#104.

@mr-tz
Copy link
Collaborator

mr-tz commented Aug 5, 2021

Very cool. Willi and I just chatted about this and will open up an discussion on how to handle the rules to avoid cross-file matching and address some other concerns. Getting the higher level rule matches for free is a great benefit of your approach!

It's really great that you provide this update back to the community to kickstart a robust handling of ELF files. Big thanks!

@williballenthin
Copy link
Collaborator

williballenthin commented Aug 5, 2021

We've begun a discussion on rule organization here: #701 and we would very much appreciate your insight.

Some of these ideas we've had for a while but didn't really address because we only supported one OS. Now, we need to figure out what we want to do.

@williballenthin
Copy link
Collaborator

My current thinking is to merge this PR (#700) and the testfile PR mandiant/capa-testfiles#104 very soon/immediately. Thoughts @mr-tz @Ana06 @mike-hunhoff?

For the rules PR mandiant/capa-rules#442 we'd like to address two things:

  1. reviews of the logic by a few people (@re-fox certainly and I'll reach out to a few other people)
  2. flesh out any rule re-organization mentioned above

(2) is not anything you're necessarily responsible for; though, this PR now makes it important for us to figure out. So, if you have the patience, we'd like to discuss and (probably) decide before merging. It'll be easier to make changes to the pending patches rather than go back and fixup after a merge. However, we realize that this may feel slower to you and we don't want to discourage further contributions. So, please let us know if you understand and/or what you'd like to see!

@TcM1911
Copy link

TcM1911 commented Aug 5, 2021

I don't have any issues with holding off the merge of the rules until the re-organization has been decided.

@williballenthin williballenthin merged commit ff08b99 into mandiant:master Aug 11, 2021
@williballenthin
Copy link
Collaborator

#723 adds support for features specifying the os/format/arch. if you have the time, @TcM1911 and @Adir-Shemesh would you take a peek and raise any concerns, etc.? if the PR passes review and gets merged, then we can tweak the ELF rules as appropriate and get the word out. hopefully by early next week or sooner (@mr-tz is on vacation at the moment).

incidentally, we're introducing breaking changes so we'll release a capa v3.0 for the ELF support. while we don't necessarily consider major releases as actually "major" its a good time for blog/tweets/etc. Would you all like to put together a blog post on the new ELF support? we'd be happy to share it around and re-post from our accounts - its important to us to recognize how awesome it was to receive these contributions from you!

@TcM1911
Copy link

TcM1911 commented Aug 18, 2021

@williballenthin, sounds good. I'll reach out to you privately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ELF support

4 participants